🐛 fix(limiter): floor sub-second expiration in sliding window - #4564
Conversation
SlidingWindow computed the expiration as uint64(expirationDuration.Seconds()), so any positive sub-second ExpirationFunc value (e.g. 500ms) truncated to 0. The window weight is then float64(resetInSec) / float64(expiration), a division by zero that makes the rate NaN, so every request — including the first — was rejected with 429 regardless of Max. Floor the truncated window to 1 second. Adds a regression test that is rejected with 429 before the fix and returns 200 after.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughLimiter expiration normalization is centralized in ChangesLimiter expiration normalization
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4564 +/- ##
==========================================
- Coverage 93.31% 93.24% -0.07%
==========================================
Files 140 140
Lines 14858 14859 +1
==========================================
- Hits 13864 13856 -8
- Misses 619 627 +8
- Partials 375 376 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@nikolauspschuetz The linter workflow is failing |
Satisfies the gocritic httpNoBody linter.
uint64(expirationDuration.Seconds()) truncates a positive sub-second ExpirationFunc value to a 0-second window. The sliding window then divides by zero, so the rate is int(NaN) and requests within Max are rejected. The fixed window rotates on every request instead, resets its counter each time and stops limiting altogether, which is the same bug failing open. Fold the non-positive fallback and the floor into one resolveExpiration helper both strategies share. The fixed window needs the floored duration too, since it hands it to the storage as the entry TTL, and a 500ms TTL under a 1s window would drop hits mid-window. The regression test only reproduced on amd64: int(NaN) wraps to MinInt64 there but saturates to 0 on arm64, where the unfixed sliding window admitted the request and the test passed. Assert X-RateLimit-Reset instead, which is 0 without the floor and 1 with it on every architecture, and run it against both strategies with an injected clock.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@middleware/limiter/config.go`:
- Around line 35-36: Align the expiration documentation with resolveExpiration:
in middleware/limiter/config.go lines 35-36, document that only positive
sub-second durations are floored to one second while zero or negative durations
use the default expiration; apply the same clarification to
docs/middleware/limiter.md lines 113-114 and update the ExpirationFunc table
description at line 135.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8e4c660f-37ac-43f2-9c53-22c2a277522b
📒 Files selected for processing (5)
docs/middleware/limiter.mdmiddleware/limiter/config.gomiddleware/limiter/limiter_fixed.gomiddleware/limiter/limiter_sliding.gomiddleware/limiter/limiter_test.go
gocritic's unnamedResult rejected the (time.Duration, uint64) pair, and naming the results would just trade it for nonamedreturns. Return the window seconds only and let the fixed window derive its storage TTL through the existing secondsToDuration helper, which keeps the TTL tied to the floored window.
The field doc and the docs page both said every value below one second floors to one second. Zero and negative values fall back to the default expiration instead, so name which case does what.
|
Congrats on merging your first pull request! 🎉 We here at Fiber are proud of you! If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
There was a problem hiding this comment.
Pull request overview
Fixes limiter window sizing when ExpirationFunc returns a positive sub-second duration, preventing zero-second windows that break sliding-window math (division by zero → NaN/Inf) and aligning behavior to a minimum 1-second accounting window.
Changes:
- Introduce
windowSecondsto floor positive sub-second expirations to a 1-second window (and keep existing fallback for non-positive durations). - Apply the new window sizing in both sliding and fixed limiter strategies.
- Add regression tests covering sub-second expirations and update limiter docs to describe the whole-second accounting behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| middleware/limiter/limiter_sliding.go | Uses windowSeconds to ensure sliding-window expiration is never 0 seconds. |
| middleware/limiter/limiter_fixed.go | Uses windowSeconds (and derives TTL from whole-second window) so fixed-window behavior is consistent for sub-second expirations. |
| middleware/limiter/config.go | Documents the behavior and adds the shared windowSeconds helper. |
| middleware/limiter/limiter_test.go | Adds regression tests for sub-second expirations for both strategies. |
| docs/middleware/limiter.md | Documents flooring behavior for ExpirationFunc in user-facing docs. |
| if d <= 0 { | ||
| d = ConfigDefault.Expiration | ||
| } | ||
| if sec := uint64(d.Seconds()); sec > 0 { |
| // Generate expiration from generator. The storage TTL is derived from the | ||
| // window so a sub-second value cannot expire the entry mid-window. | ||
| expiration := windowSeconds(cfg.ExpirationFunc(c)) | ||
| expirationDuration, _ := secondsToDuration(expiration) |
Description
SlidingWindowcomputes the window length as:Any positive sub-second
ExpirationFuncvalue truncates to0. The window weight is thenso
ratebecomes garbage and every request — including the first — is rejected with429, regardless ofMax. A limiter configured with, say,ExpirationFunc: func(fiber.Ctx) time.Duration { return 500*time.Millisecond }blocks all traffic.Fix
Floor the truncated window to 1 second (a sub-second window is treated as a 1-second window rather than breaking):
Testing
Added
TestLimiterSlidingSubSecondExpiration, which is rejected with429before this change and returns200after.go test ./middleware/limiter/(full package),go vet, andgofmtare clean.Note:
FixedWindowshares the sameuint64(...Seconds())truncation; I scoped this PR toSlidingWindowsince that's the one with a clean, demonstrable failure (the NaN-rate 429). Happy to follow up on the fixed-window window-sizing behavior separately if maintainers agree it's worth addressing.